More strict commit policy for stable branch#11609
More strict commit policy for stable branch#11609aszlig wants to merge 5 commits intoNixOS:masterfrom
Conversation
The motivation behind this was a cherry-pick of c204036 to the release branch, which broke evaluation. Now I've cherry-picked another commit which was needed by c204036 (58e9440) because otherwise evaluation would fail. I by myself didn't state a reason at the time where I was cherry-picking the commit but figured that I should have done so after being noted by @lethalman about a related work that removes the changes in the commit I have cherry-picked. Long story short: If we cherry-pick stuff to any of the release branches, we should explicitly state the reasons *why* we did so. Not only because reviewers spend less time wondering about the introduced changes but to give the committer time to rethink about whether it really *is* necessary to include the change to a supposedly stable branch. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
aszlig
added
9.needs: reporter feedback
|
By analyzing the blame information on this pull request, we identified @cillianderoiste and @mcmtroffaes to be potential reviewers |
|
Cc: @NixOS/nixpkgs-committers |
|
👍 |
1 similar comment
|
👍 |
doc/submitting-changes.xml
Outdated
There was a problem hiding this comment.
We don't need Cherry-picked-from because it's already provided by git cherry-pick -x (though I see submitting-changes.xml doesn't mention -x, so we should probably add a note that -x is required). So it would be something like:
(cherry picked from commit 54d6f1f683a448e094f31b5732c424c2467eaf8d)
Reason: bla bla bla
Also, providing a reason is redundant if the original message has an issue link (e.g. Fixes #123).
There was a problem hiding this comment.
Well, I was using Cherry-picked-from because it looks more consistent.
If the original message has an issue link it doesn't necessarily mean that it's a justification for cherry-picking it to stable.
I was using Cherry-picked-from and Cherry-pick-reason because it looked more consistent in the commit message. But as @edolstra mentioned, git cherry-pick -x already provides a message, which would be inconsistent of course. However, looking at it from a more "big picture"ish perspective, the "(cherry picked from commit ...)" is a standard Git message so it probably is a better idea to leave it like that for consistency with other cherry picks. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
|
As long as we're discussing this, shouldn't there be an actual policy written in that document for what could or should be cherry-picked to the stable branch vs what shouldn't? |
Another suggestion by @edolstra. Most of the people already did this when cherry-picking to stable, but with this note it's now official :-) Signed-off-by: aszlig <aszlig@redmoonstudios.org>
This makes it a bit easier to find the rules that apply to a particular branch only, apart from the rules that apply for all branches. In the long term this is to structure the documentation so that we also have a full textual descriptions instead of just itemized lists. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
|
@wizeman: That's actually a good point. I mean security and bug fixes may sound obvious, but even that isn't so clear if you give it a second thought, for example: Do you just add a patch or update to the newest upstream version? What about mass-rebuild changes? And what about changes that are not security/bug fixes? What about changes to NixOS modules? |
|
Maybe we could agree to adhere to http://chris.beams.io/posts/git-commit/ |
|
@DamienCassou this article is nice, but it's over plus for us IMO. |
doc/submitting-changes.xml
Outdated
There was a problem hiding this comment.
The documentation could also suggest to use the -e flag so the commit message can be edited, i.e.
<command>git cherry-pick -x -e</command>
There was a problem hiding this comment.
Also, the sentence is relatively hard to read for me. Let me suggest
If you're cherry-picking a commit to a stable release branch,
always use <command>git cherry-pick -xe</command>
and ensure the message contains a clear description about
why this needs to be included in the stable branch.
|
I think that the best start is to by example. Find agreement with a list of good commits to stable, and a list of bad Please use actual commits that happened; there is no need to write against Regards, On Thu, Dec 10, 2015 at 09:33:52PM -0800, Arseniy Seroka wrote:
|
Improved wording by @vcunat, thanks a lot. I personally always amend commits after cherry-picking, which has grown into a bit of a habit and why I didn't notice the missing -e (suggested by @mcmtroffaes) in the first place. This is of course hereby added as well. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
|
@DamienCassou: I already am quite "nazi" about commit messages and I'm already committing almost as in the linked article. However I do put a period at the end of the subject (will avoid that from now on because I didn't realize that I always OCDed it in). So from my point of view, I'd strongly recommend making that the default commit policy so that we at least can try to encourage people to do so. |
|
👍 |
|
Squashed and pushed (while leaving authorship with Aszlig). |
12 participants
The motivation behind this was a cherry-pick of c204036 to the 15.09 release branch, which broke evaluation. Now I've cherry-picked another commit which was needed by c204036 (58e9440) because otherwise evaluation would fail.
I by myself didn't state a reason at the time where I was cherry-picking the commit but figured that I should have done so after being noted by @lethalman about a related work that removes the changes in the commit I have cherry-picked.
Long story short: If we cherry-pick stuff to any of the release branches, we should explicitly state the reasons why we did so.
Not only because reviewers spend less time wondering about the introduced changes but to give the committer time to rethink about whether it really is necessary to include the change to a supposedly
stable branch.